Add client steps implementation to support existing functionalities#6966
Add client steps implementation to support existing functionalities#6966alfonso-noriega wants to merge 10 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
Coverage report
Test suite run success3981 tests passing in 1526 suites. Report generated by 🧪jest coverage report action from a6262e1 |
9c2e1fd to
7e48497
Compare
16768f3 to
59f0139
Compare
7e48497 to
159bb73
Compare
159bb73 to
e6f3577
Compare
packages/app/src/cli/services/build/steps/include_assets_step.ts
Outdated
Show resolved
Hide resolved
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 2 findings 📋 History❌ Failed → ✅ 1 findings → ❌ Failed → ❌ Failed → ✅ 1 findings → ✅ 2 findings |
bbc0817 to
7b68eb7
Compare
e6f3577 to
3f28156
Compare
3f28156 to
44034c5
Compare
f9f9e63 to
59dcaa7
Compare
44034c5 to
094a68c
Compare
59dcaa7 to
fb4c01d
Compare
0478573 to
a8b39c0
Compare
a872825 to
9688bf3
Compare
bc8f04a to
35aaabb
Compare
9688bf3 to
d42767e
Compare
35aaabb to
906c43e
Compare
isaacroldan
left a comment
There was a problem hiding this comment.
Review assisted by pair-review
packages/app/src/cli/services/build/steps/copy-static-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include_assets_step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include_assets_step.ts
Outdated
Show resolved
Hide resolved
…pattern copy - Add sanitizeDestination() that strips '..' segments from destination fields and emits a warning when any are removed - Sanitize entry.destination for all three inclusion types (pattern, static, configKey) before it reaches any path join - Add copy-time bounds check in copyByPattern: skip any file whose resolved destPath escapes outputDir and warn Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
- Replace positional args with config objects in copyByPattern, copySourceEntry, and copyConfigKeyEntry to prevent argument-order mistakes (e.g. mixing include/ignore patterns) - Make the static dispatch branch explicit with if (entry.type === 'static') - Fix copySourceEntry: when destination is provided and source is a directory, use copyDirectoryContents + glob count instead of copyFile + hardcoded 1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
packages/app/src/cli/services/build/steps/include-assets-step.ts
Outdated
Show resolved
Hide resolved
…type
- Change StaticEntrySchema preserveStructure default from false to true
so directory sources preserve their name in the output by default
rather than silently merging contents into the output root
- Unwrap copyByPattern return from {filesCopied: number} to number,
consistent with copySourceEntry and copyConfigKeyEntry
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract sanitizeDestination into sanitizeRelativePath in @shopify/cli-kit/node/path so it can be shared across the codebase - Simplify copySourceEntry by resolving destPath and logMsg up front then dispatching once on file vs directory, eliminating the duplicated copyDirectoryContents + glob + return count pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move each copy strategy into its own file under include-assets/: - copy-by-pattern.ts — glob-based file selection - copy-source-entry.ts — explicit source path copy - copy-config-key-entry.ts — config key resolution + nested path helpers include-assets-step.ts is now a thin orchestrator holding only schemas and executeIncludeAssetsStep. No behavior change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
counts is (number | undefined)[] due to TypeScript not seeing the three if-checks as exhaustive. Explicit generic <number> on reduce fixes it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Direct tests for copyByPattern, copySourceEntry, and copyConfigKeyEntry covering edge cases not exercised through the orchestrator tests: collision deduplication, path traversal guard, filepath===destPath no-op, all destination resolution branches, file vs directory sources, destination param, nested [] flatten paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/path.d.ts import type { URL } from 'url';
/**
* Joins a list of paths together.
*
* @param paths - Paths to join.
* @returns Joined path.
*/
export declare function joinPath(...paths: string[]): string;
/**
* Normalizes a path.
*
* @param path - Path to normalize.
* @returns Normalized path.
*/
export declare function normalizePath(path: string): string;
/**
* Resolves a list of paths together.
*
* @param paths - Paths to resolve.
* @returns Resolved path.
*/
export declare function resolvePath(...paths: string[]): string;
/**
* Returns the relative path from one path to another.
*
* @param from - Path to resolve from.
* @param to - Path to resolve to.
* @returns Relative path.
*/
export declare function relativePath(from: string, to: string): string;
/**
* Returns whether the path is absolute.
*
* @param path - Path to check.
* @returns Whether the path is absolute.
*/
export declare function isAbsolutePath(path: string): boolean;
/**
* Returns the directory name of a path.
*
* @param path - Path to get the directory name of.
* @returns Directory name.
*/
export declare function dirname(path: string): string;
/**
* Returns the base name of a path.
*
* @param path - Path to get the base name of.
* @param ext - Optional extension to remove from the result.
* @returns Base name.
*/
export declare function basename(path: string, ext?: string): string;
/**
* Returns the extension of the path.
*
* @param path - Path to get the extension of.
* @returns Extension.
*/
export declare function extname(path: string): string;
/**
* Parses a path into its components (root, dir, base, ext, name).
*
* @param path - Path to parse.
* @returns Parsed path object.
*/
export declare function parsePath(path: string): {
root: string;
dir: string;
base: string;
ext: string;
name: string;
};
/**
* Given an absolute filesystem path, it makes it relative to
* the current working directory. This is useful when logging paths
* to allow the users to click on the file and let the OS open it
* in the editor of choice.
*
* @param path - Path to relativize.
* @param dir - Current working directory.
* @returns Relativized path.
*/
export declare function relativizePath(path: string, dir?: string): string;
/**
* Given 2 paths, it returns whether the second path is a subpath of the first path.
*
* @param mainPath - The main path.
* @param subpath - The subpath.
* @returns Whether the subpath is a subpath of the main path.
*/
export declare function isSubpath(mainPath: string, subpath: string): boolean;
/**
* Given a module's import.meta.url it returns the directory containing the module.
*
* @param moduleURL - The value of import.meta.url in the context of the caller module.
* @returns The path to the directory containing the caller module.
*/
export declare function moduleDirectory(moduleURL: string | URL): string;
/**
* When running a script using `npm run`, something interesting happens. If the current
* folder does not have a `package.json` or a `node_modules` folder, npm will traverse
* the directory tree upwards until it finds one. Then it will run the script and set
* `process.cwd()` to that folder, while the actual path is stored in the INIT_CWD
* environment variable (see here: https://docs.npmjs.com/cli/v9/commands/npm-run-script#description).
*
* @returns The path to the current working directory.
*/
export declare function cwd(): string;
/**
* Tries to get the value of the `--path` argument, if provided.
*
* @param argv - The arguments to search for the `--path` argument.
* @returns The value of the `--path` argument, if provided.
*/
export declare function sniffForPath(argv?: string[]): string | undefined;
/**
* Returns whether the `--json` or `-j` flags are present in the arguments.
*
* @param argv - The arguments to search for the `--json` and `-j` flags.
* @returns Whether the `--json` or `-j` flag is present in the arguments.
*/
export declare function sniffForJson(argv?: string[]): boolean;
+/**
+ * Removes any `..` traversal segments from a relative path and calls `warn`
+ * if any were stripped. Normal `..` that cancel out within the path (e.g.
+ * `foo/../bar` → `bar`) are collapsed but never allowed to escape the root.
+ * Both `/` and `\` are treated as separators for cross-platform safety.
+ *
+ * @param input - The relative path to sanitize.
+ * @param warn - Called with a human-readable warning when traversal segments are removed.
+ * @returns The sanitized path (may be an empty string if all segments were traversal).
+ */
+export declare function sanitizeRelativePath(input: string, warn: (msg: string) => void): string;
|

Add build step infrastructure for extension asset management
WHY are these changes introduced?
This introduces a flexible build step system to handle various asset copying and processing needs for different extension types, replacing hardcoded build logic with configurable steps.
WHAT is this pull request doing?
'admin'to theCONFIG_EXTENSION_IDSarray for admin extensionsexecuteIncludeAssetsStep- handles pattern-based, static, and config-key-driven asset copying with comprehensive test coverageexecuteCopyStaticAssetsStep- copies static assets defined in extension build manifestsexecuteCreateTaxStubStep- generates minimal JavaScript stub files for tax calculation extensionsexecuteStepByType) that dispatches to appropriate handlers based on step typeHow to test your changes?
include_assetsstep type with different inclusion strategiesinclude-assets-step.test.tsMeasuring impact
How do we know this change was effective? Please choose one:
Checklist